Skip to content

[wgpu-hal] gles/wgl: Migrate from ancient/unmaintained winapi to windows-rs #6006

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
teoxoy merged 1 commit into from
Jul 25, 2024
Merged

[wgpu-hal] gles/wgl: Migrate from ancient/unmaintained winapi to windows-rs #6006

teoxoy merged 1 commit into from
Jul 25, 2024

Conversation

@MarijnS95
Copy link
Contributor

@MarijnS95 MarijnS95 commented Jul 21, 2024 •
edited
Loading

Connections
Contributes to #3207
Same as PR #5956 but for GLES/WGL, instead of the Dx12 backend.

Description
Replaces ancient, deprecated, unmaintained winapi crate with windows-rs. The full justification is in the linked issue and PR above.

Testing

WGPU_BACKEND=gles cargo run --bin wgpu-examples shadow

Checklist

  • Run cargo fmt.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
    • --target wasm32-unknown-emscripten
  • Run cargo xtask test to run tests.
  • Add change to CHANGELOG.md. See simple instructions inside file.

@MarijnS95 MarijnS95 requested a review from a team as a code owner July 21, 2024 20:21
@MarijnS95 MarijnS95 force-pushed the gles-wgl-windows-rs branch from 7437069 to 04d45c0 Compare July 21, 2024 20:24
Wumpf
Wumpf approved these changes Jul 22, 2024
View reviewed changes
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff, thank you so much for taking care of the windows-rs migration everywhere!

Imho good to go after the workspace dependency nit (see comment)

@MarijnS95 MarijnS95 force-pushed the gles-wgl-windows-rs branch from 04d45c0 to 264f81c Compare July 22, 2024 09:06
Wumpf
Wumpf reviewed Jul 22, 2024
View reviewed changes
cwfitzgerald
cwfitzgerald requested changes Jul 22, 2024
View reviewed changes
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment above

@cwfitzgerald
Copy link
Member

cwfitzgerald commented Jul 23, 2024

I suspect this windows version dance is going to be a recurring theme in our future. That is one upside off using a years-abandoned crate, nothing ever changes 😛

Anywho I think we (the maintainers) should figure out what the viable paths forward are that satisfy mozilla-central and wgpu's external users and then clearly communicate that, as I don't want Marijn to do duplicate work and I don't want this to explode mozilla-central.

  • Ideally we can figure out a way to land 0.57/58. It's not improbable that nothing else in the stack breaks and mozilla-central can just update, though I know there will be politics around that
  • if not, dropping back and supporting as wide of a range as we can, to minimize duplicate dependencies. We should earmark Marjin's improvements, so when et do update, we can re-implement them.
  • loom is indeed irrelevant
  • if there is no range beyond 0.52

@Wumpf Wumpf mentioned this pull request Jul 23, 2024
Merged
6 tasks
glandium
glandium approved these changes Jul 23, 2024
View reviewed changes
Copy link
Contributor

@glandium glandium left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem for me to upgrade to windows 0.58.

@MarijnS95
Copy link
Contributor Author

MarijnS95 commented Jul 24, 2024

@cwfitzgerald since we've made the resolution to just go ahead and use windows 0.58 directly (it seems), can we get reviews finished? Landing this PR first will make it much easier and cleaner to rebase #5956 on top.

@MarijnS95 MarijnS95 changed the title gles/wgl: Migrate from ancient/unmaintained winapi to windows-rs [wgpu-hal] gles/wgl: Migrate from ancient/unmaintained winapi to windows-rs Jul 24, 2024
cwfitzgerald
cwfitzgerald approved these changes Jul 24, 2024
View reviewed changes
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff! Got the all clear to merge from the moz team.

@cwfitzgerald
Copy link
Member

cwfitzgerald commented Jul 24, 2024

Ready to land, just needs merge conflicts resolved

@MarijnS95 MarijnS95 force-pushed the gles-wgl-windows-rs branch from 264f81c to 3c8785b Compare July 24, 2024 20:22
@MarijnS95 MarijnS95 force-pushed the gles-wgl-windows-rs branch from 3c8785b to e3a7c6c Compare July 24, 2024 20:32
@teoxoy teoxoy merged commit 2611d18 into gfx-rs:trunk Jul 25, 2024
@MarijnS95 MarijnS95 deleted the gles-wgl-windows-rs branch July 25, 2024 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Wumpf Wumpf Wumpf approved these changes

@cwfitzgerald cwfitzgerald cwfitzgerald approved these changes

@ErichDonGubler ErichDonGubler Awaiting requested review from ErichDonGubler

@waywardmonkeys waywardmonkeys Awaiting requested review from waywardmonkeys

+1 more reviewer

@glandium glandium glandium approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@MarijnS95 @cwfitzgerald @waywardmonkeys @ErichDonGubler @glandium @Wumpf @teoxoy